Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ui): updates to dark mode logic #3740

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zackbcom
Copy link
Contributor

@zackbcom zackbcom commented Jun 2, 2024

Changes

  • Updates to dark mode logic to have 3 options, System defined, Dark and, Light.
  • Reused bool in the settings to use null as system defined.
  • Created listener for changes to system dark mode. Works on mac when selecting system theme.
  • Added button toggle to support the 3 cases.

TODO

  • Need to figure out what to do with the login checkbox.
    • Might be able to look into a indeterminate state for system.
    • Replace with button stack
    • Remove and use system default, update in settings.

Testing Artifacts

  • After
    • image

@coveralls
Copy link

coveralls commented Jun 3, 2024

Pull Request Test Coverage Report for Build 9362246394

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 21.163%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stores/base.js 0 1 0.0%
Totals Coverage Status
Change from base Build 9347582370: 0.0%
Covered Lines: 3899
Relevant Lines: 19593

💛 - Coveralls

@@ -666,6 +669,8 @@ export default {
hideTopbar: false,
title: '',
messages: [],
systemDark: false,
mq_system_dark: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't use a watch for this instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it's not clear to me what mq_system_dark is used for, you never re-assign it, I would instead use a let in beforeMount instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time using Vue. Will re look at the logic here. Thanks for the notes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok no worries I appreciate a lot your help 🙏🏼

Comment on lines +1632 to +1639
beforeUnmount() {
if (this.mq_system_dark !== null) {
this.mq_system_dark.removeEventListener(
'change',
this.updateSystemDark,
)
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit useless as the App will only be unmounted when the page is destroyed

@zackbcom
Copy link
Contributor Author

I don't have time right now to work on this.

@zackbcom zackbcom closed this Jul 10, 2024
@robertsLando
Copy link
Member

@zackbcom No worries, keep it open for now will see if I can find some time to look at it :)

@robertsLando robertsLando reopened this Jul 10, 2024
@coveralls
Copy link

coveralls commented Jul 10, 2024

Pull Request Test Coverage Report for Build 11250226051

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 21.021%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/stores/base.js 0 1 0.0%
Totals Coverage Status
Change from base Build 11230985389: 0.0%
Covered Lines: 3925
Relevant Lines: 19852

💛 - Coveralls

Copy link
Contributor

github-actions bot commented Oct 9, 2024

This pull-request is stale because it has been open 90 days with no activity. Remove the stale label or comment or this will be closed in 5 days. To ignore this pull-request entirely you can add the no-stale label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants